Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add helpful error message for failed julia dependency imports #2359

Conversation

JacksonBurns
Copy link
Contributor

Motivation or Problem

Currently when RMG tries to import the Julia dependencies, a failed import is simply ignored due to testing limitations.

Description of Changes

Issues a warning if the import of the Julia dependencies fails, but only when not in the testing mode.

Testing

I built an environment without the Julia dependencies properly built to verify that the warning would be issued.

Reviewer Tips

The message should be more helpful, maybe pointing toward the install directions or some relevant GitHub issues.

@JacksonBurns
Copy link
Contributor Author

I had intended this to catch the exception but specified the wrong type, it should be catching a julia.core.UnsupportedPythonError instead of an ImportError. I'm not sure if we will be able to get to the former since the import breaks inside nose tests, leaving us with the option of making the except bare? Not great practice but maybe all that's possible here.

@codecov
Copy link

codecov bot commented Jan 22, 2023

Codecov Report

Merging #2359 (11279b4) into main (856f72a) will decrease coverage by 0.43%.
The diff coverage is 0.00%.

❗ Current head 11279b4 differs from pull request most recent head 2521e39. Consider uploading reports for the commit 2521e39 to get more accurate results

@@            Coverage Diff             @@
##             main    #2359      +/-   ##
==========================================
- Coverage   48.12%   47.69%   -0.43%     
==========================================
  Files         110      110              
  Lines       30654    30518     -136     
  Branches     7994     7952      -42     
==========================================
- Hits        14753    14557     -196     
- Misses      14365    14444      +79     
+ Partials     1536     1517      -19     
Impacted Files Coverage Δ
rmgpy/rmg/reactors.py 19.43% <0.00%> (-1.31%) ⬇️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

since this issue will only cause an exception to be raised later on during execution, it makes more sense to raise a more helpful exception here
this is a barebones file that currently only tests the new import logic. more importantly, it demonstrates that we may be able to add more test coverage with clever use of patching and magicmock
@JacksonBurns
Copy link
Contributor Author

I welcome input on whether this should raise a warning or an error, as well as what the content of the message should be.

I have also added a test file that bypasses the issue with nosetests and rms being incompatible. The python docs provide a great example on patching sys.modules that I have used here.

@JacksonBurns JacksonBurns changed the title add helpful warning message for failed julia dependency imports add helpful error message for failed julia dependency imports Jan 30, 2023
@JacksonBurns
Copy link
Contributor Author

...sigh, this was working on my local install. Will keep looking at it.

@JacksonBurns JacksonBurns added the Status: WIP This is currently work-in-progress label Feb 1, 2023
@JacksonBurns
Copy link
Contributor Author

This will be resolved in #2443

@JacksonBurns JacksonBurns deleted the julia-import-warning branch May 18, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: WIP This is currently work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant